[Camera] Improved resolution presets by mklim · Pull Request #1952 · flutter/plugins

mklim · 2019-08-07T01:32:54Z

Description

This continues the work started in #1403 by @quentinleguennec.

This PR is here to address issues in the quality of video recording for the camera plugin on Android, as well as discrepancies in quality level between Android and iOS. It also allows to control the still picture quality (which was so far only maximum quality). The existing resolution presets have been updated to match between Android and iOS, and 2 new presets have been added (veryHigh and veryLow).

There are no breaking changes but the quality of the presets have been adjusted which will result in different picture/video quality on apps using this plugin.

We believe this change is necessary as, for the same resolution preset, Android video quality was very poor and the quality on iOS was to high, generating 4k files that are too heavy to upload.

Related Issues

#1186
#1107
flutter/flutter#30459
flutter/flutter#29500
flutter/flutter#26959

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

…xed uncaught exception.

Only works on Android since it requires automatic accepting of camera permissions to run.

googlebot · 2019-08-07T01:33:01Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

packages/camera/android/src/main/java/io/flutter/plugins/camera/Camera.java

bparrishMines · 2019-08-07T22:15:17Z

packages/camera/android/src/main/java/io/flutter/plugins/camera/CameraUtils.java

-      }
+  static Size computeBestPreviewSize(String cameraName, ResolutionPreset preset) {
+    if (preset.ordinal() > ResolutionPreset.high.ordinal()) {
+      preset = ResolutionPreset.high;


Why do you set the preset to high here?

i think it's that high res previews negatively impact performance. The current code in master is setting previewSize to be the absolute minimum resolution that's the correct aspect ratio, and is also capped at 1080 (line 37 in the original patch).

Maybe you could improve to let the choice to the user here. My current use case need a stream at the highest resolution. And without this clamp, the function startPreviewWithImageStream works perfectly.

It would be acceptable to set the default to high or even medium. But this change actually cripples the powerful Android devices that default to 1080p video even at 60fps.

bparrishMines · 2019-08-07T22:22:27Z

packages/camera/example/pubspec.yaml

    sdk: flutter
+  flutter_driver:
+    sdk: flutter
+  test: 1.6.3


Is this dependency actually used? I believe you only use flutter_test?

It was extra, removed.

bparrishMines · 2019-08-07T22:24:52Z

packages/camera/example/test_driver/camera.dart

+  enableFlutterDriverExtension(handler: (_) => completer.future);
+
+  setUpAll(() async {
+    final Directory extDir = await getApplicationDocumentsDirectory();


Wouldn't it be better to use getTemporaryDirectory so that you wouldn't have to delete them all each time?

getTemporaryDirectory() still needs to be deleted by us but that does make more sense. Changed.

bparrishMines · 2019-08-07T22:32:15Z

packages/camera/example/test_driver/camera.dart

+        final bool presetExactlySupported =
+            await testCaptureImageResolution(controller, preset.key);
+        assert(
+            !(!previousPresetExactlySupported &&


This is always true. Did you mean to use presetExactlySupported.

Whoops, yes.

bparrishMines · 2019-08-07T22:36:43Z

packages/camera/example/test_driver/camera.dart

+        final bool presetExactlySupported =
+            await testCaptureVideoResolution(controller, preset.key);
+        assert(
+            !(!previousPresetExactlySupported &&


Same as above. Always true.

bparrishMines · 2019-08-07T22:40:27Z

packages/camera/pubspec.yaml

+    sdk: flutter
+  flutter_driver:
+    sdk: flutter
+  test: 1.6.3


nit: Is this dependency used?

googlebot · 2019-08-07T22:59:38Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

mklim · 2019-08-07T23:03:28Z

Manually verified the CLAs since the author gave permission for these commits to be contributed to the project in the original PR.

googlebot · 2019-08-07T23:16:27Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-07T23:25:20Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

bparrishMines

LGTM

This PR is here to address issues in the quality of video recording for the camera plugin on Android, as well as discrepancies in quality level between Android and iOS. It also allows to control the still picture quality (which was so far only maximum quality). The existing resolution presets have been updated to match between Android and iOS, and new presets have been added. There are no breaking changes but the quality of the presets have been adjusted which will result in different picture/video quality on apps using this plugin. We believe this change is necessary as, for the same resolution preset, Android video quality was very poor and the quality on iOS was too high, generating 4k files that are too heavy to upload.

quentinleguennec and others added 10 commits March 28, 2019 10:23

Added camcorderProfile to Android for better quality preset selection.

9a774a8

Improved best profile selection algorithm.

1232d22

Camera: Now resolutionPreset also influence image capture quality. Fi…

39c2d0e

…xed uncaught exception.

Use medium resolution preset for the example.

dcac87d

Fixed merge conflict with version 0.4.3+2.

018521d

Fixed formating.

945dc88

Merge branch 'master' into tengio_improved_camera_plugin

0972b28

Add integration test for size settings

5f482cc

Only works on Android since it requires automatic accepting of camera permissions to run.

Review feedback and format

d376121

Merge branch 'master' into tengio_improved_camera_plugin

875b28e

mklim requested a review from bparrishMines August 7, 2019 01:32

googlebot added the cla: no label Aug 7, 2019

mklim mentioned this pull request Aug 7, 2019

[Camera] Improved resolution presets and matched quality between Android and iOS #1403

Closed

13 tasks

Fix formatting issues.

5d07729

bparrishMines reviewed Aug 7, 2019

View reviewed changes

mklim added cla: yes and removed cla: no labels Aug 7, 2019

Review fixup

efc525a

googlebot added cla: no and removed cla: yes labels Aug 7, 2019

mklim added cla: yes and removed cla: no labels Aug 7, 2019

bparrishMines self-requested a review August 8, 2019 00:05

bparrishMines approved these changes Aug 8, 2019

View reviewed changes

mklim merged commit d515868 into flutter:master Aug 8, 2019

mklim deleted the tengio_improved_camera_plugin branch August 8, 2019 15:27

bparrishMines mentioned this pull request Aug 27, 2019

Future of the Camera Plugin (Refactor/Rework) flutter/flutter#31225

Closed

bparrishMines mentioned this pull request Sep 16, 2019

[camera] Improve CameraPreview to size itself appropriate for video and picture previews flutter/flutter#15953

Open

This was referenced Sep 24, 2019

Flutter Camera plugin captures video rotated 90 degrees when in Landscape on iPhone flutter/flutter#29951

Closed

[camera] camera preview rotates in landscape mode flutter/flutter#39669

Closed

mvanbeusekom mentioned this pull request Jan 4, 2021

Flutter camera plugin record quality is very poor flutter/flutter#26959

Closed

stuartmorgan-g mentioned this pull request May 4, 2021

[camera] Limit camera preview size to take high quality picture with startImageStream #3843

Closed

11 tasks

comlhj1114 mentioned this pull request May 6, 2021

[camera] Take picture with max resolution #3854

Closed

11 tasks

Conversation

mklim commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

googlebot commented Aug 7, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Aug 7, 2019

Uh oh!

mklim commented Aug 7, 2019

Uh oh!

googlebot commented Aug 7, 2019

Uh oh!

googlebot commented Aug 7, 2019

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mklim commented Aug 7, 2019 •

edited

Loading